naked functions: respect function-sections#147811
naked functions: respect function-sections#147811folkertdev wants to merge 3 commits intorust-lang:mainfrom
function-sections#147811Conversation
|
Some changes occurred in compiler/rustc_codegen_ssa |
function-sections
2ebd197 to
ac4b179
Compare
ac4b179 to
6580a5f
Compare
| if let Some(section) = &link_section { | ||
| writeln!(begin, ".pushsection {section},\"xr\"").unwrap() | ||
| } else if function_sections { | ||
| writeln!(begin, ".pushsection .text${asm_name},\"xr\"").unwrap() |
There was a problem hiding this comment.
On -msvc targets, function_sections is ignored. .text$sym is only used on -gnu targets.
There was a problem hiding this comment.
Also LLVM currently generates the following line on COFF targets:
.section {section},"xr",one_only,{sym},unique,0`
Should we be doing the same here?
There was a problem hiding this comment.
On -msvc targets, function_sections is ignored.
Meaning that it's just always assumed to be on? https://godbolt.org/z/a6ofW49YK
.text$symis only used on -gnu targets.
It should still work for msvc, no?
Should we be doing the same here?
I don't think we can reliably emit the unique,id bit of that line because how do we make that ID?
There was a problem hiding this comment.
Actually using -Zfunction-sections=no on msvc does have an effect. So I'm not really sure what it being ignored means then. Maybe this is a more recent LLVM change?
There was a problem hiding this comment.
No, it's always assumed to be off on msvc. You can see it always uses .text.
There was a problem hiding this comment.
It does always use .text but normally it emits the line you quoted:
.section .text,"xr",one_only,foo,unique,0
i.e. it uses a subsection which, as far as I understand, does allow DCE, with -Zfunction-sections=no it emits just
.text
So then DCE is impossible
There was a problem hiding this comment.
The unique,0 bit is apparently an LLVM extension https://llvm.org/docs/Extensions.html#id2. It is documented as elf-specific, but clearly it's used for COFF as well...
I just don't see how we can generate the unique ID in a reliable way. Also because we would generate a function per section, the function name (which should be unique?) should be sufficient to disambiguate the sections.
In short, I think the implementation in this PR is the best we can reliably do.
|
☔ The latest upstream changes (presumably #148305) made this pull request unmergeable. Please resolve the merge conflicts. |
6580a5f to
b12830c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like windows will also be using function sections by default soon #148669 |
For `gnu` function_sections is off by default.
b12830c to
77de006
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This is unlikely to happen, as you can see crashes in failed tests in that PR. That is what I was talking about in #147789 (comment) |
|
Hi @folkertdev, are you waiting for another review or is the discussion thread here still active? |
|
In my mind this was blocked on review, but now that I read it again I should give this a once-over and probably test both with and without @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
|
Hi @Amanieu, it looks like this is ready for another round of review when you have a chance 🙂 |
|
@bors r+ |
…, r=Amanieu naked functions: respect `function-sections` fixes rust-lang#147789 r? @Amanieu
…uwer Rollup of 7 pull requests Successful merges: - #147811 (naked functions: respect `function-sections`) - #153834 (Merge `fabsf16/32/64/128` into `fabs::<F>`) - #154043 (simd_fmin/fmax: make semantics and name consistent with scalar intrinsics) - #154494 (triagebot: add reminder for bumping CI LLVM stamp) - #153374 (Fix LegacyKeyValueFormat report from docker build: dist-x86_64) - #154320 (`trim_prefix` for paths) - #154453 (Fix ice in rustdoc of private reexport)
|
This pull request was unapproved. This PR was contained in a rollup (#154514), which was unapproved. |
| #[unsafe(naked)] | ||
| #[no_mangle] | ||
| extern "C" fn unused() { | ||
| naked_asm!("ret") | ||
| } | ||
|
|
||
| #[unsafe(naked)] | ||
| #[link_section = "foobar"] | ||
| #[no_mangle] | ||
| extern "C" fn unused_link_section() { | ||
| naked_asm!("ret") | ||
| } |
There was a problem hiding this comment.
@bjorn3 does it make sense to you that these functions are not removed at link time on windows? They are not present on linux at least, but with windows they are. So either we're missing something or the windows linker just keeps these functions despite them being unreachable.
There was a problem hiding this comment.
I bet they'd get removed before the changes in this PR. Since function sections are disabled for windows-gnu, beginning with this PR they are no longer output in separate sections, so ld.bfd doesn't GC them.
There was a problem hiding this comment.
In that case I'd expect -Zfunction-sections=yes to cause them to be removed again, but that is not what I see. In fact even unused_link_section is not removed, and it always has its own section.
I'm using cross-compilation here so maybe the issue is there? But it looks like unused sections just aren't removed. The same is true when I use normal (i.e. non-naked) functions.
There was a problem hiding this comment.
You are right they aren't removed fully. Cross-compilation should be irrelevant.
Before this PR or with -Zfunction-sections=yes nm shows them as undefined:
❯ nm main.exe | rg unused
U unused
U unused_link_section
With this PR but without -Zfunction-sections=yes only unused_link_section is undefined:
❯ nm main.exe | rg unused
0000000140001540 T unused
U unused_link_section
Linking the same code with LLD (-Clink-arg=-fuse-ld=lld) doesn't remove them at all:
❯ nm main-lld.exe | rg unused
000000014009a230 T unused
00000001400d5000 T unused_link_section
In fact LLD didn't even discard foobar section.
The same is true when I use normal (i.e. non-naked) functions.
With GNU ld yes, but with LLD the normal function gets removed. If I add:
#[no_mangle]
extern "C" fn unused_regular() {}unused_regular is defined without function sections, made undefined with function sections, and removed with LLD + function sections:
Details
❯ nm main.exe | rg '\sused|unused'
U unused
U unused_link_section
0000000140001630 T unused_regular
000000014009a150 T used
❯ RUSTC_BOOTSTRAP=1 rustc --target x86_64-pc-windows-gnu main.rs -Zfunction-sections=yes
❯ nm main.exe | rg '\sused|unused'
U unused
U unused_link_section
U unused_regular
000000014009a148 T used
❯ RUSTC_BOOTSTRAP=1 rustc --target x86_64-pc-windows-gnu main.rs -Clink-arg=-fuse-ld=lld
❯ nm main.exe | rg '\sused|unused'
000000014009a230 T unused
00000001400d5000 T unused_link_section
0000000140001630 T unused_regular
000000014009a234 T used
❯ RUSTC_BOOTSTRAP=1 rustc --target x86_64-pc-windows-gnu main.rs -Clink-arg=-fuse-ld=lld -Zfunction-sections=yes
❯ nm main.exe | rg '\sused|unused'
000000014009a230 T unused
00000001400d5000 T unused_link_section
000000014009a234 T used
Quite a mess nonetheless.
There was a problem hiding this comment.
Thanks for that detective work!
With GNU ld yes, but with LLD the normal function gets removed
So, that is different from the unused naked function, right? Are you able to identify what the difference is?
Regardless, what should we test for windows here. Like obviously we could ignore it entirely, that would be simplest. Alternatively we could add regular function equivalents and assert that the naked function is treated the same as a non-naked function?
There was a problem hiding this comment.
So, that is different from the
unusednaked function, right?
Correct.
Are you able to identify what the difference is?
COMDAT usage for regular function is the most striking difference to me:
❯ llvm-readobj -Sst main.o | less
...
Section {
Number: 4
Name: .text$unused (2F 31 31 35 00 00 00 00)
...
Characteristics [ (0x60300020)
IMAGE_SCN_ALIGN_4BYTES (0x300000)
IMAGE_SCN_CNT_CODE (0x20)
IMAGE_SCN_MEM_EXECUTE (0x20000000)
IMAGE_SCN_MEM_READ (0x40000000)
]
}
...
Section {
Number: 22
Name: .text$unused_regular (2F 32 34 00 00 00 00 00)
...
Characteristics [ (0x60501020)
IMAGE_SCN_ALIGN_16BYTES (0x500000)
IMAGE_SCN_CNT_CODE (0x20)
IMAGE_SCN_LNK_COMDAT (0x1000)
IMAGE_SCN_MEM_EXECUTE (0x20000000)
IMAGE_SCN_MEM_READ (0x40000000)
]
}
With C example:
❯ cat main.c
void used() {}
void unused() {}
int main() { used(); return 0; }build with x86_64-w64-mingw32-gcc main.c -Wl,--gc-sections -ffunction-sections -fno-asynchronous-unwind-tables I'm seeing similar thing.
With GCC unused function is undefined with GNU ld and defined with LLD, but with Clang while GNU ld behaves the same way and LLD removes the symbol.
Once again this seems to boil down to IMAGE_SCN_LNK_COMDAT (0x1000) that is set for the intermediary object only with Clang.
Perhaps memory fails me but I think GNU ld used to remove such symbols in the past. Anyway for this PR this solution seems nice if it doesn't add too much work:
Alternatively we could add regular function equivalents and assert that the naked function is treated the same as a non-naked function?
CI only tests regular x86_64-pc-windows-gnu which gives us no function sections and GNU ld as the linker, so in both cases the symbols will be undefined (assuming that the older ld installed on CI also doesn't get rid of the symbol).
With x86_64-pc-windows-gnullvm target this test would fail because regular function would be removed. But since it is not run on the CI, this is actually a good thing showing a potential for improvement.
View all comments
fixes #147789
r? @Amanieu